Summary

• Fixes AI losing all conversation context when a queued message interrupts a streaming response
• The previous fix (6e246a0) cleared sessionId on abort to avoid resuming a dead session, but this caused the next message to start a completely fresh session with zero prior context since the Claude Code SDK relies solely on session ID-based resumption
• The session file on disk (~/.claude/) is still valid after abort — the actual issue was resumeSessionAt pointing to an incomplete message UUID, not the session itself

Changes

• Keep sessionId on abort instead of nulling it (both save paths in claude.ts)
• Guard sdkMessageUuid so it's not set on aborted streams — prevents resumeSessionAt from targeting an incomplete message
• Remove sessionInvalidated flag from client transport — no longer needed since server preserves the session
• Remove cancel mutation fallback that cleared sessionId from DB when no active controller existed
• Existing isSessionNotFound handler acts as safety net for genuinely stale sessions

Test plan

• Start a multi-turn conversation so there's meaningful context
• While the AI is streaming, type and send a new message (triggers queued message flow)
• Verify the AI's response to the queued message still has context of the prior conversation
• Verify normal (non-interrupted) conversations still work correctly
• Verify stopping a response and then sending a new message also preserves context

@1code review this pr

Task completed.

Now I have a good understanding of the changes. Let me provide my review.

PR Review: #126 - Fix Queued Messages Lost When Clicking "Send Now" During Streaming

Overview

This PR addresses a significant bug where clicking "Send Now" on a queued message while the AI was streaming would cause the message to be lost entirely. The fix also addresses a related session ID race condition.

Code Quality Assessment

Overall: Approve with minor suggestions

Strengths

1. Event-driven approach over polling (active-chat.tsx:225-256)
   The waitForStreamingReady() helper using Zustand's subscribe is cleaner than the previous setTimeout polling loop. The 30-second safety timeout is a good defensive measure.

2. Error recovery with requeue (active-chat.tsx:3580-3584)
   The try-catch wrapper around handleSendFromQueue with prependItem ensures messages aren't lost if sending fails — this is the core fix and it's well-implemented.

3. Editor content restoration (active-chat.tsx:3728-3731)
   Similarly, handleForceSend now restores editor content on failure, so users don't lose their typed message.

4. Fixed arePropsEqual memoization (chat-input-area.tsx:184-186, 214-215)
   Adding queueLength, firstQueueItemId, and onSendFromQueue to the equality check fixes stale closure bugs in the memoized component.

5. Cleanup of dead code
   Removing the fetch('/api/agents/chat') DELETE calls is correct — there's no REST server in the Electron app.

Issues & Suggestions

1. Session context handling discrepancy (Medium priority)

The PR description mentions two different behaviors:

• In active-chat.tsx:3534 comment: "server-side save block preserves sessionId on abort"
• In active-chat.tsx:3631 comment: "server-side save block sets sessionId=null on abort"

Looking at the actual claude.ts changes, the code preserves sessionId (the second commit reverted the nulling behavior). But the comment at line 3631 is stale/incorrect:

// The server-side save block sets sessionId=null on abort, so the next
// message starts fresh without needing an explicit cancel mutation.

This comment contradicts the actual behavior. It should read similarly to line 3534.

2. sdkMessageUuid guard condition (Low priority)

At claude.ts:1507, the guard && !abortController.signal.aborted prevents setting sdkMessageUuid on aborted streams. This is correct because resuming at an incomplete message would cause issues. However, it might be worth adding a brief comment explaining why this guard exists for future maintainers.

3. Cancel mutation return value change (Low priority)

At claude.ts:1968-1970, the return logic changed:

// Before: return { cancelled: true } inside if, { cancelled: false } outside
// After:  return { cancelled: !!controller } at the end

This is functionally equivalent but slightly changes behavior: if controller existed but abort() somehow failed silently, it would still return cancelled: true. Not a real issue, just noting the semantic change.

4. Removed cancel mutation call (Verify intended)

In ipc-chat-transport.ts:447, the trpcClient.claude.cancel.mutate() call was removed from the abort handler. The comment in claude.ts cleanup handler says session cleanup is handled server-side, but verify this works correctly when:

• User clicks stop rapidly multiple times
• Network hiccup during abort

Since the server-side abort is triggered by abortController.abort() in the unsubscribe callback, this should be fine, but worth verifying in testing.

Testing Recommendations

The test plan in the PR is comprehensive. Additionally, I'd suggest testing:

1. Rapid queue operations: Queue 2-3 messages, then rapidly click "Send Now" on different items
2. Network failure simulation: What happens if the send fails due to network issues? Does prependItem work correctly?
3. App restart mid-stream: Stop the app while streaming, restart — verify session can be resumed

Summary

Verdict: Approve — The core bug fix is solid and the improvements to error handling are welcome. The stale comment at active-chat.tsx:3631 should be updated to match the actual behavior (sessionId is preserved, not nulled).

View full conversation

Powered by 1Code

merged